GH-36795: [C#] Implement support for dense and sparse unions #36797

CurtHagenlocher · 2023-07-21T01:03:57Z

What changes are included in this PR?

Support dense and sparse unions in the C# implementation.
Adds Archery support for C# unions.

Are these changes tested?

Yes

Are there any user-facing changes?

Unions are now supported in the C# implementation.

This PR includes breaking changes to public APIs.

The public APIs for the UnionArray and UnionType were changed fairly substantially. As these were previously not implemented properly, the impact of the changes ought to be minimal.

The ChunkedArray and Column classes were changed to hold IArrowArrays instead of Arrays. To accomodate this, a constructor was added which may introduce ambiguity in calling code. This could be avoided by changing the overloaded constructor to instead be a factory method. This didn't seem worthwhile but could be reconsidered.

The metadata version was finally increased to V5.

Closes: [C#] Implement support for dense and sparse unions #36795

… Union

github-actions · 2023-07-21T01:04:29Z

⚠️ GitHub issue #36795 has been automatically assigned in GitHub to PR creator.

lidavidm · 2023-09-05T15:40:13Z

csharp/src/Apache.Arrow/C/CArrowSchemaExporter.cs

+            {
+                UnionMode.Sparse => "+us:",
+                UnionMode.Dense => "+ud:",
+                _ => throw new InvalidDataException($"Unsupported time unit for export: {unionType.Mode}"),


Suggested change

_ => throw new InvalidDataException($"Unsupported time unit for export: {unionType.Mode}"),

_ => throw new InvalidDataException($"Unsupported union mode for export: {unionType.Mode}"),

lidavidm · 2023-09-05T17:01:56Z

csharp/src/Apache.Arrow/Arrays/DenseUnionArray.cs

+            : base(data)
+        {
+            ValidateMode(UnionMode.Dense, Type.Mode);
+            data.EnsureBufferCount(2); // TODO: 


maybe link this TODO to a new issue? (and what is the TODO about, given SparseUnionArray lacks one?)

Heck if I can remember... . Will just remove the TODO.

lidavidm · 2023-09-05T17:04:01Z

Hmm, it seems C# producing unions fails in the integration test, though

…Union

CurtHagenlocher · 2023-09-06T13:36:20Z

Hmm, it seems C# producing unions fails in the integration test, though

I thought this had worked before, but it clearly can't have. There may be more than one problem, but the obvious one is the exception thrown when comparing the data generated from the JSON template with the checked-in data. The template for union test data contains duplicate names -- two fields named "struct" and two fields named "dense". One of each field is marked as nullable in the JSON and one as non-nullable. But the data file that's been checked-in shows all four fields as being non-nullable. I'm not sure what to make of this. I don't see code in the Java or C++ implementations which special-case anything about nullability -- either in reading from the JSON template or in the implementation classes themselves.

lidavidm · 2023-09-06T14:14:41Z

Ah...right before 1.0 unions were changed: they no longer have a top-level nullability. I guess the files may not have been changed to reflect this.

CurtHagenlocher · 2023-09-06T15:07:20Z

I still don't understand how this is working for the other implementations. I've looked at the code for Field and for the Field comparers and for the JSON import and there's no special-case which says "ignore the nullability bit on Field if this is a Union". I'll have to come back to it later.

CurtHagenlocher · 2023-09-10T18:45:46Z

It looks like some changes made to Archery in the last week have fixed the errors we were getting. I'm ... not entirely comfortable with that, I guess, but I spent a lot of time trying to understand if/what the C# implementation was doing differently without any success.

Edit: nope, looks like one problem solved, one still remains.

lidavidm · 2023-09-11T12:04:25Z

Likely b957847? Which does now explicitly generate union fields with/without nullability to check what happens.

…Union

lidavidm · 2023-09-19T18:33:51Z

The Java error makes it sound like C# is still writing with metadata version 4:

arrow/format/Schema.fbs

Lines 38 to 48 in fa43106

    
           /// >= 0.8.0 (December 2017). Non-backwards compatible with V3. 
        
           V4, 
        
           /// >= 1.0.0 (July 2020. Backwards compatible with V4 (V5 readers can read V4 
        
           /// metadata and IPC messages). Implementations are recommended to provide a 
        
           /// V4 compatibility mode with V5 format changes disabled. 
        
           /// 
        
           /// Incompatible changes between V4 and V5: 
        
           /// - Union buffer layout has changed. In V5, Unions don't have a validity 
        
           ///   bitmap buffer. 
        
           V5,

That may also explain things, if it's triggering some compatibility path, e.g.:

arrow/cpp/src/arrow/ipc/reader.cc

Lines 391 to 413 in fa43106

    
           // With metadata V4, we can get a validity bitmap. 
        
           // Trying to fix up union data to do without the top-level validity bitmap 
        
           // is hairy: 
        
           // - type ids must be rewritten to all have valid values (even for former 
        
           //   null slots) 
        
           // - sparse union children must have their validity bitmaps rewritten 
        
           //   by ANDing the top-level validity bitmap 
        
           // - dense union children must be rewritten (at least one of them) 
        
           //   to insert the required null slots that were formerly omitted 
        
           // So instead we bail out. 
        
           if (out_->null_count != 0 && out_->buffers[0] != nullptr) { 
        
             return Status::Invalid( 
        
                 "Cannot read pre-1.0.0 Union array with top-level validity bitmap"); 
        
           } 
        
           out_->buffers[0] = nullptr; 
        
           out_->null_count = 0; 
        
           if (out_->length > 0) { 
        
             RETURN_NOT_OK(GetBuffer(buffer_index_, &out_->buffers[1])); 
        
             if (type.mode() == UnionMode::DENSE) { 
        
               RETURN_NOT_OK(GetBuffer(buffer_index_ + 1, &out_->buffers[2])); 
        
             } 
        
           }

… Union

CurtHagenlocher · 2023-09-23T14:17:56Z

Thanks @lidavidm for your help; couldn't have made it work without you!

conbench-apache-arrow · 2023-09-25T18:25:01Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit e55f912.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…pache#36797) ### What changes are included in this PR? Support dense and sparse unions in the C# implementation. Adds Archery support for C# unions. ### Are these changes tested? Yes ### Are there any user-facing changes? Unions are now supported in the C# implementation. **This PR includes breaking changes to public APIs.** The public APIs for the UnionArray and UnionType were changed fairly substantially. As these were previously not implemented properly, the impact of the changes ought to be minimal. The ChunkedArray and Column classes were changed to hold IArrowArrays instead of Arrays. To accomodate this, a constructor was added which may introduce ambiguity in calling code. This could be avoided by changing the overloaded constructor to instead be a factory method. This didn't seem worthwhile but could be reconsidered. The metadata version was finally increased to V5. * Closes: apache#36795 Authored-by: Curt Hagenlocher <[email protected]> Signed-off-by: David Li <[email protected]>

CurtHagenlocher added 7 commits July 19, 2023 20:47

Initial changes for Union support

dc253c7

Union-related fixes

1bb1417

Merge branch 'apache:main' into Union

56df33a

Fixed IPC to work correctly for unions

6b3ff65

Merge branch 'Union' of https://github.com/CurtHagenlocher/arrow into…

01eb95b

… Union

Implement Archery support for C# unions

c576eca

Better backwards compatibility

a113a80

CurtHagenlocher requested review from assignUser, kou, raulcd and westonpace as code owners July 21, 2023 01:03

github-actions bot added Component: C# Component: Documentation awaiting review Awaiting review labels Jul 21, 2023

CurtHagenlocher added 6 commits July 23, 2023 13:41

Merge branch 'main' of https://github.com/apache/arrow into Union

bfc26ee

Merge branch 'main' of https://github.com/apache/arrow into Union

2bd6f3d

Merge from main

f892aba

Merge from main and resolve conflicts

b77d04c

Fix deserialization of fixed-size list

039ec9e

Fix bug in ctor

0327be2

davidhcoe mentioned this pull request Sep 3, 2023

feat(csharp): Add ADO.NET client; tests for C# to interop with the Snowflake Go driver apache/arrow-adbc#1031

Merged

lidavidm added the Breaking Change Includes a breaking change to the API label Sep 4, 2023

lidavidm approved these changes Sep 5, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Sep 5, 2023

CurtHagenlocher added 2 commits September 5, 2023 21:00

PR feedback

832f7cb

Merge branch 'main' of https://github.com/CurtHagenlocher/arrow into …

6591b81

…Union

Merge from main

0de1117

Merge branch 'main' of https://github.com/CurtHagenlocher/arrow into …

a29e1c1

…Union

davidhcoe mentioned this pull request Sep 21, 2023

[Java] Arrow Flight Core doesn't build in container #37827

Closed

CurtHagenlocher added 4 commits September 22, 2023 16:10

Increment metadata version to V5 and add handling for V4 unions.

5ebefcf

Merge branch 'main' into Union

ddac4d3

Correctly skip buffer in pre-V5 metadata

143a469

Merge branch 'Union' of https://github.com/CurtHagenlocher/arrow into…

3ef09de

… Union

lidavidm approved these changes Sep 25, 2023

View reviewed changes

lidavidm merged commit e55f912 into apache:main Sep 25, 2023
13 checks passed

lidavidm removed the awaiting merge Awaiting merge label Sep 25, 2023

CurtHagenlocher deleted the Union branch September 25, 2023 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36795: [C#] Implement support for dense and sparse unions #36797

GH-36795: [C#] Implement support for dense and sparse unions #36797

CurtHagenlocher commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023

lidavidm Sep 5, 2023

lidavidm Sep 5, 2023

CurtHagenlocher Sep 6, 2023 •

edited

Loading

lidavidm commented Sep 5, 2023

CurtHagenlocher commented Sep 6, 2023

lidavidm commented Sep 6, 2023

CurtHagenlocher commented Sep 6, 2023

CurtHagenlocher commented Sep 10, 2023 •

edited

Loading

lidavidm commented Sep 11, 2023

lidavidm commented Sep 19, 2023

CurtHagenlocher commented Sep 23, 2023

conbench-apache-arrow bot commented Sep 25, 2023

	_ => throw new InvalidDataException($"Unsupported time unit for export: {unionType.Mode}"),
	_ => throw new InvalidDataException($"Unsupported union mode for export: {unionType.Mode}"),

GH-36795: [C#] Implement support for dense and sparse unions #36797

GH-36795: [C#] Implement support for dense and sparse unions #36797

Conversation

CurtHagenlocher commented Jul 21, 2023 • edited Loading

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jul 21, 2023

lidavidm Sep 5, 2023

Choose a reason for hiding this comment

lidavidm Sep 5, 2023

Choose a reason for hiding this comment

CurtHagenlocher Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

lidavidm commented Sep 5, 2023

CurtHagenlocher commented Sep 6, 2023

lidavidm commented Sep 6, 2023

CurtHagenlocher commented Sep 6, 2023

CurtHagenlocher commented Sep 10, 2023 • edited Loading

lidavidm commented Sep 11, 2023

lidavidm commented Sep 19, 2023

CurtHagenlocher commented Sep 23, 2023

conbench-apache-arrow bot commented Sep 25, 2023

CurtHagenlocher commented Jul 21, 2023 •

edited

Loading

CurtHagenlocher Sep 6, 2023 •

edited

Loading

CurtHagenlocher commented Sep 10, 2023 •

edited

Loading